Skip to content

Conversation

AndreiKingsley
Copy link
Collaborator

No description provided.

@AndreiKingsley AndreiKingsley marked this pull request as ready for review September 30, 2025 10:27
@Jolanrensen
Copy link
Collaborator

image

Very nice and very useful!

Copy link
Collaborator

@Jolanrensen Jolanrensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the API, it's really useful! I do have some comments

* A lambda used to format a column header (its displayed name) when rendering a dataframe to HTML.
*
* The lambda runs in the context of [FormattingDsl] and receives the [ColumnWithPath] of the header to format.
* Return a [CellAttributes] (or `null`) describing CSS you want to apply to the header cell.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*the CSS

* Return a [CellAttributes] (or `null`) describing CSS you want to apply to the header cell.
*
* Examples:
* - Center a header: `attr("text-align", "center")`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*the header

* Examples:
* - Center a header: `attr("text-align", "center")`
* - Make it bold: `bold`
* - Set custom color: `textColor(rgb(10, 10, 10))`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*a custom color

// region DataFrame.formatHeader

/**
* **Experimental API. It may be changed in the future.**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to introduce an Experimental OptIn for this API?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe kdoc note is enough.

*/
@Suppress("UNCHECKED_CAST")
public fun <T, C> HeaderFormatClause<T, C>.with(formatter: HeaderColFormatter<C>): FormattedFrame<T> {
val selectedPaths = df.getColumnPaths(UnresolvedColumnsPolicy.Skip, columns).toSet()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe move this to formatHeaderImpl

val composedHeader: HeaderColFormatter<Any?> = { col ->
val path = col.path
// Merge attributes from selected parents
val parentAttributes = if (path.size > 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait... so you're merging attributes from parents here, but also in html.kt? how does it work?

if (hf == null) {
null
} else {
// collect attributes from parents
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you also do this in formatHeader.kt?

var nestedRowsLimit: Int? = 5,
var cellContentLimit: Int = 40,
var cellFormatter: RowColFormatter<*, *>? = null,
var headerFormatter: HeaderColFormatter<*>? = null,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a public data class; you're breaking binary compatibility by adding this, especially halfway

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had the same issue with ParserOptions :/ unfortunately, using data classes in the public api is a bit of a nightmare. Adding a new argument breaks the constructor, componentX(), and copy() function.
Check out ParserOptions for a workaround for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants